lots of changed lines but its mostly just adding a level of nesting - I didnt change any existing tests. just added the disabled describe block, which ensures the correct telemetry service implementation is used (real one or noop) according to whether telemetry is enabled.

Feel like I need to elaborate on this approach because it might deviate from the expectations in the issue a bit which said:

In order to still respect the ENABLE_TELEMETRY and ENABLE_TELEMETRY_TRACES flags, all we can do is just to not register metric/trace providers in the telemetry initialization.

What we're doing here is adding a new implementation that just doesnt do anything and using that if telemetry is disabled.

I can revise this if we don't like it but I think its working well.

I didn't do it as described for a couple reasons: 1) I don't think its possible - we arent registering metric/trace providers on service init. Traces are already registered outside the service so they seem to be a non-factor here. 2) I think even if we did we'd find ourselves in a scenario where we're calling methods like incrementCounterWithTransactionAmount, doing all that work (including network call) and then simply do nothing.

Alternatives considerd:

1. in the spirit of the original suggestion, we could simply gate the counter create in getOrCreateCounter (and histogram) behind if telemetry is enabled and returned undefined if disabled like:

  private getOrCreateCounter(name: string, options?: MetricOptions): Counter {
    let counter = this.counters.get(name)
    if (!counter || !deps.enableTelemetry) {
      counter = metrics.getMeter(METER_NAME).createCounter(name, options)
      this.counters.set(name, counter)
    }
    return counter
  }

Then we just use optional chaining to do nothing if undefined:

  const counter = this.getOrCreateCounter(name)
  counter.?add(...)

But again, this has the problem of doing all the work in incrementCounterWithTransactionAmount for example only to not actually do anything at the end. We'd also need to remember to apply the same pattern if we added additional metrics beyond counter/histograms.

2. Just add an early return to recordHistogram/incrementCounter if telemetry is disabled. But would the person adding the next similar method remember to do the same? One reason why I think have it at the highest level (as it is in createTelemetryService is good).

1. I don't think its possible - we arent registering metric/trace providers on service init

I see what you mean, what happens to the existing metrics/counters/histogramts when we keep the original/existing telemetry service, but with enableTelemetry disabled?

But again, this has the problem of doing all the work in incrementCounterWithTransactionAmount for example only to not actually do anything at the end

I mean, it might not be a terrible thing, I don't think it really does much when there are no providers configured

I see what you mean, what happens to the existing metrics/counters/histogramts when we keep the original/existing telemetry service, but with enableTelemetry disabled?

I tested this after writing these comments and it's a no-op since src/telemetry/index.ts wont register the meter. So no metrics collected, same as this. We can just go that direction if we're OK with executing the real telemetry service methods where they're called. Honestly this shouldnt be a problem except we are doing more work than simply passing the values into the metric counter/histrogram in the convert methods. Potential network call and applying privacy.

Maybe its not the biggest deal, and presumably new methods shouldn't do any more than this...

I don't think it really does much when there are no providers configured

telemetry meter providers? In src/telemetry/index? I'm not seeing how thats a factor. I think it runs the whole method with rate conversion, privacy, etc. and but never gets kicked up to prometheus because the meter isnt registered.

Yes, I'm with you, having a No-op class is clearer, and we don't end up having any side effects (even if they don't really do anything).

what do these do?

removes a lot of noise from the collector logs. watching the container logs with telemetry enabled is unreadable without it.

Yes, I'm with you, having a No-op class is clearer, and we don't end up having any side effects (even if they don't really do anything).

I think I prefer having this if (!deps.enableTelemetry) check in src/index.ts instead of here, since it would seem clearer to me to understand how each class/dependency gets registered (kind of like tigerbeetle). Also wouldn't need to pass in enableTelemetry: boolean. NABD, just an option.

that makes sense - good to have the switch as high level as possible I think.

Changed to return the noop or real one based on config. enableTelemetry in src/index.ts. In doing so I realized we don't need to pass in all the deps to the noop one so I removed those as well.